Fix DirectoryNotFoundException when deleting cache twice #3992
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a DirectoryNotFoundException that occurs when attempting to clean the cache twice by adding a check to verify the plugin cache directory exists before attempting to enumerate and delete its contents.
- Adds existence check for plugin cache directory before enumeration
- Refactors variable usage to reuse existing pluginCacheDirectory reference
- Wraps cache deletion logic in conditional block to prevent exception
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs
Outdated
Show resolved
Hide resolved
|
🥷 Code experts: Yusyuriv, onesounds Jack251970, onesounds have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
|
@coderabbitai full review |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds an existence guard to ClearCacheFolder in SettingsPaneAboutViewModel to skip deletion when the plugin cache directory is missing, retains per-subdirectory deletion with logging, deletes the cache directory when present, and always raises CacheFolderSize property change to refresh UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant VM as SettingsPaneAboutViewModel
participant FS as FileSystem
User->>VM: ClearCacheFolder()
VM->>FS: Check pluginCacheDirectory.Exists
alt Directory exists
VM->>FS: Enumerate subdirectories
loop For each subdirectory
VM->>FS: Delete subdirectory (try/catch + log)
end
VM->>FS: Delete pluginCacheDirectory (non-recursive, try/catch + log)
else Directory missing
Note over VM: Skip deletion steps
end
VM->>VM: OnPropertyChanged("CacheFolderSize")
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs (1)
234-266: Good fix; add try/catch around enumeration to avoid TOCTOU crash.Even with the Exists check, the directory can be removed between the check and EnumerateDirectories(), causing DirectoryNotFoundException (or UnauthorizedAccessException). Wrap the whole enumeration + final Delete in try/catch and log; also prefer FullName in logs.
- // Check if plugin cache directory exists before attempting to delete - // Or it will throw DirectoryNotFoundException in `pluginCacheDirectory.EnumerateDirectories` - if (pluginCacheDirectory.Exists) - { - // Firstly, delete plugin cache directories - pluginCacheDirectory.EnumerateDirectories("*", SearchOption.TopDirectoryOnly) - .ToList() - .ForEach(dir => - { - try - { - // Plugin may create directories in its cache directory - dir.Delete(recursive: true); - } - catch (Exception e) - { - App.API.LogException(ClassName, $"Failed to delete cache directory: {dir.Name}", e); - success = false; - } - }); - - // Then, delete plugin directory - var dir = pluginCacheDirectory; - try - { - dir.Delete(recursive: false); - } - catch (Exception e) - { - App.API.LogException(ClassName, $"Failed to delete cache directory: {dir.Name}", e); - success = false; - } - } + // Check if plugin cache directory exists before attempting to delete. + // Guard against TOCTOU by wrapping enumeration + delete. + try + { + if (pluginCacheDirectory.Exists) + { + // Firstly, delete plugin cache directories + pluginCacheDirectory.EnumerateDirectories("*", SearchOption.TopDirectoryOnly) + .ToList() + .ForEach(dir => + { + try + { + // Plugin may create directories in its cache directory + dir.Delete(recursive: true); + } + catch (Exception e) + { + App.API.LogException(ClassName, $"Failed to delete cache directory: {dir.FullName}", e); + success = false; + } + }); + + // Then, delete plugin directory (should now be empty) + try + { + pluginCacheDirectory.Delete(recursive: false); + } + catch (Exception e) + { + App.API.LogException(ClassName, $"Failed to delete cache directory: {pluginCacheDirectory.FullName}", e); + success = false; + } + } + } + catch (DirectoryNotFoundException) + { + // Swallow: directory disappeared between Exists check and enumeration. + } + catch (UnauthorizedAccessException e) + { + App.API.LogException(ClassName, $"Access denied enumerating plugin cache directory: {pluginCacheDirectory.FullName}", e); + success = false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (1)
Flow.Launcher/SettingPages/ViewModels/SettingsPaneAboutViewModel.cs (1)
268-269: Unconditional CacheFolderSize notification — LGTM.Matches the discussion to keep the UI accurate even if the folder was manually removed.
Fix DirectoryNotFoundException when deleting cache twice
…twice Fix DirectoryNotFoundException when deleting cache twice
CHANGES
If we click
Clean cachebutton, it will throwDirectoryNotFoundExceptioninpluginCacheDirectory.EnumerateDirectories. We should check ifpluginCacheDirectoryexists before callingEnumerateDirectoriesfunction.TEST
Click
Clean cachebutton twice.